Skip to content

Harden rollover fee accumulation#2380

Open
TUPM96 wants to merge 2 commits into
Lightprotocol:mainfrom
TUPM96:harden-rollover-fee-overflow
Open

Harden rollover fee accumulation#2380
TUPM96 wants to merge 2 commits into
Lightprotocol:mainfrom
TUPM96:harden-rollover-fee-overflow

Conversation

@TUPM96
Copy link
Copy Markdown

@TUPM96 TUPM96 commented May 25, 2026 •
edited by coderabbitai Bot
Loading

Summary

  • use checked addition when accumulating rollover fees for the same account index
  • propagate arithmetic overflow instead of allowing unchecked wrapping
  • add a regression test for overflow handling

Tests

  • cargo test -p light-system-program-pinocchio --tests
  • cargo clippy -p light-system-program-pinocchio --tests -- -D warnings

Summary by CodeRabbit

  • Bug Fixes

    • Rollover fee updates now detect and report arithmetic overflow instead of silently overflowing.
    • Error propagation improved so failures setting rollover fees are returned and handled.

  • Tests

    • Added unit test covering overflow during rollover fee updates to ensure state integrity.

Review Change Stack

Copilot AI review requested due to automatic review settings May 25, 2026 09:28
@TUPM96 TUPM96 requested a review from ananas-block as a code owner May 25, 2026 09:28
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 49285a3e-478c-4076-99a4-65d4e5109909

📥 Commits

Reviewing files that changed from the base of the PR and between 0823295 and 8ff4760.

📒 Files selected for processing (1)
  • programs/system/src/context.rs
📝 Walkthrough

Walkthrough

This PR makes rollover-fee accumulation overflow-safe: SystemContext::set_rollover_fee returns Result<()> and uses checked_add to detect overflow (yielding ProgramError::ArithmeticOverflow); two processor callers now propagate that error with ?.

Changes

Rollover Fee Overflow Safety

Layer / File(s) Summary
Rollover fee accumulation with overflow safety
programs/system/src/context.rs		 set_rollover_fee now returns Result<()> and uses checked_add to detect arithmetic overflow, mapping it to ProgramError::ArithmeticOverflow. A test-only new_for_test() constructor and unit test verify overflow behavior when the same index is updated repeatedly.
Error propagation in processor callers
programs/system/src/processor/create_address_cpi_data.rs, programs/system/src/processor/create_outputs_cpi_data.rs		 Both derive_new_addresses and create_outputs_cpi_data now use ? to propagate errors from set_rollover_fee, turning previously-ignored failures into early returns with the underlying error.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

ai-review

Suggested reviewers

  • sergeytimoshin
  • ananas-block
  • SwenSchaeferjohann

Poem

🧮 Tiny fees on ledger's track,
Add with care, don't let them stack.
When totals reach the tipping line,
Trap the overflow, return the sign.
Safety first — arithmetic fine.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately reflects the main change: hardening rollover fee accumulation through checked arithmetic and error propagation to prevent overflow.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 70.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI reviewed May 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds checked arithmetic and error propagation when accumulating rollover fees to prevent silent u64 overflow, plus a unit test covering the overflow case.

Changes:

  • Make SystemContext::set_rollover_fee return Result<()> and use checked_add with ProgramError::ArithmeticOverflow on overflow.
  • Update CPI data builders to propagate set_rollover_fee errors (?).
  • Add a unit test and a test-only constructor for SystemContext.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
programs/system/src/processor/create_outputs_cpi_data.rs Propagates rollover-fee accumulation failures when building output CPI data.
programs/system/src/processor/create_address_cpi_data.rs Propagates rollover-fee accumulation failures when deriving new address CPI data.
programs/system/src/context.rs Implements checked fee accumulation with an overflow error, and adds a regression test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread programs/system/src/context.rs Outdated
}

pub fn set_rollover_fee(&mut self, ix_data_index: u8, fee: u64) {
pub fn set_rollover_fee(&mut self, ix_data_index: u8, fee: u64) -> Result<()> {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@ananas-block ananas-block Awaiting requested review from ananas-block ananas-block is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@TUPM96